perf(inno): keep the native LZMA2 finish buffer instead of copying it - #278
Conversation
The adapter's queue() copied every buffer the native decoder handed back. That is right for update(), which the library documents as returning a zero-copy view, but finish() resolves to the decoded tail of a decoder that is spent afterwards, so nothing can rewrite it. On a solid block that copy was a second copy of the whole block.
Zaldaryon
left a comment
There was a problem hiding this comment.
Approved. The adapter keeps the defensive copy for every update() result and skips it only for the final finish() tail, after the native decoder has no further calls to make. The pending queue still preserves output order and limits each callback to 2 MiB. Native codec failures still use the existing TypeScript fallback.\n\nThe new adapter test models the relevant ownership contract: update() reuses one buffer, finish() overwrites that buffer before returning its final tail, and the assembled output remains correct. The existing Inno extraction test also exercises the native decoder with the LZMA2 fixture.\n\nLocal verification passed: the affected suites passed with 14 tests and 1 skipped, typecheck, lint with 0 errors and 15 existing warnings, format check, 1634 tests with 2 skipped, coverage at 92.58% statements / 89.81% branches / 92.03% functions / 94.05% lines, and build:unpack. GitHub typecheck, lint, test, SonarCloud, Ubuntu build, and Windows build checks also pass.
Summary
queue()insrc/ipc/workers/nativeLzma2.tscopied every buffer the native decoder handed back, throughUint8Array.from. This keeps that copy on theupdate()path and drops it on thefinish()path, which on a solid block is where the whole decompressed payload arrives.The two paths are not the same, and the library says so itself.
@napi-rs/lzma@1.5.1documentsLzma2Decompressor.update()inindex.d.tsas returning "the bytes decoded so far (possibly empty) as a zero-copy view". A view is exactly the thing that can be written over by a later call, so the copy there is load bearing and stays.finish()is documented as "Signal EOF and resolve to the decoded tail off the JS thread. Idempotency-guarded: a second call rejects cleanly." It is not described as a view, and the decompressor is spent once it resolves, so there is no later call that could rewrite those bytes. The README and the package's own example say nothing more precise than that, so the asymmetry in the typings is what the change rests on: the ambiguity is only onupdate(), andupdate()keeps its copy.The diff is the
ownedflag onqueue()and one call site.Type
Checklist
dev, notmain.npm run typecheckpasses.npm run lint:cipasses.npm run format:checkpasses.npm run test:coveragepasses, coverage at or above the floor invitest.config.ts.npm run build:unpackpasses.Testing
The new test.
tests/ipc/nativeLzma2Adapter.test.tsgains a case that drives the adapter with a decompressor whoseupdate()hands back views into one reused four-byte buffer and whosefinish()overwrites that buffer before resolving to a separate tail. Output arrives across threeupdate()calls plus thefinish()tail, and the test asserts the assembled bytes. It is a real trap rather than a shape check: deleting the copy from theupdate()path turns it red, with the third chunk coming out as255,255,255,255where3,3,3,3was expected, because that chunk is still sitting in the pending queue whenfinish()runs.Memory, measured again, and the number is smaller than the one recorded in #241. Same method as that PR's third round: 400 MB of moderately compressible data compressed into one solid LZMA2 stream of 30.4 MB, driven through the same pump shape the domain uses, peak resident set read from the kernel's
VmHWMand sampled after everydecodeChunkcall. Five runs per variant, on Linux x64 with Node 22.22.finish()So this buys about 120 MB and a little wall clock, not the drop from 997 MB to 579 MB that the third review round recorded. I would rather correct that here than repeat it. Instrumenting the adapter shows why: the process sits at 220 MB when
finish()is called and is already at 888 MB by the time it resolves, before the adapter has queued anything at all. The counters confirm the copy was worth removing, 411 MB of the 419 MB total comes back fromfinish()and only 8 MB throughupdate(), but the high-water mark is set inside the library while it assembles that tail, and the adapter's copy landed after the library had released its own working memory rather than on top of it. Skipping it shaves the tail of the curve rather than half of it. What is still needed to get the native path near the TypeScript path's flat 141 MB is step 2 of the issue, the streaming variant, which bounds the whole thing to a chunk.All 27 committed
.binfixtures, both paths. Rebuilt the dual-path digest comparison: each fixture throughrunInnoExtractiontwice, once with@napi-rs/lzma/lzma2loadable and once with its import forced to throw, hashing every file in the output tree and folding those into one digest per fixture, alongside the verdict,filesWrittenandbytesWritten. All 27 identical, refusal messages included. The two runs really were different paths: the native run reports the factory loaded, the disabled run reports it unavailable, and counting the calls whereisNativeLzma2Errorreturned true gives zero, so no fixture quietly fell back to TypeScript and matched itself.Mutations. Putting the copy back on the
finish()path leaves the whole suite green and only moves the memory numbers, which is expected, since nothing observable changes for a caller. The mutation that matters is the other one: removing the copy from theupdate()path fails the new case, as above.Gates on
13d0ce5.typecheckclean.lint:ci0 errors and 15 warnings, all pre-existing rendererreact-hooks/exhaustive-depsones.format:checkclean.test:coverage137 files, 1,634 passed and 2 skipped, at 92.58% statements, 89.81% branches, 92.03% functions, 94.05% lines, against floors of 87, 85, 85 and 89 invitest.config.ts.build:unpackpasses on Linux x64.Related issues
This is step 1 of #265. Step 2 there, the streaming variant, stays open: it replaces the adapter's buffering model outright and wants its own change.